-
Notifications
You must be signed in to change notification settings - Fork 5
build: make zarr dependency optional #112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thanks for this, and the PR over at ome-zarr models! I'm very pro having an optional dependency on zarr-python in both places, if it's possible. My immediate question is how are you envisaging using this library without |
pydantic-zarr essentially defines a bidirectional mapping from zarr hierarchies to JSON documents, and zarr-python is just one of several possible ways to handle the translation. eventually, an API like this would be great: # use zarr python
GroupSpec.to_zarr(engine='zarr-python')
# use tensorstore
GroupSpec.to_zarr(engine='tensorstore')
# use a custom zarr engine
GroupSpec.to_zarr(engine=MyCustomZarrEngine) But the return type of |
yeah, the mapping to/from json documents that @d-v-b mentioned is what I’m after. And the use case is https://github.com/pymmcore-plus/ome-writers … which is a library for writing both ome-tiff and ome-zarr as data comes off the microscope. We have alternative backends: for zarr it’s either tensorstore or acquire-zarr (high performance libs that are both much faster than zarr-python). Having a model that simplifies the process of constructing valid, spec-conforming json documents, without stipulating exactly how those documents will be written to disk, is what I’m after. |
So the idea is you would manually create one of the models, and then serialise it to JSON using the methods available through pydantic ( |
Yes exactly.👍 Edit: for what it’s worth, if this |
- name: Run Tests (without zarr) | ||
if: matrix.zarr-version == 'none' | ||
run: | | ||
hatch run test-base.py${{ matrix.python-version }}:list-env | ||
hatch run test-base.py${{ matrix.python-version }}:test-cov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@d-v-b, let me know if you have a better idea for doing the hatch matrix here... or if you're happy to switch this repo over to using uv
and do it similarly to ome-zarr-models/ome-zarr-models-py#280
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uv would be fine by me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but that can be in a later PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall to me - I left some comments/suggestions inline.
It would also be worth making numpy
optional too I think, but that can be done in a follow up PR if you don't fancy it here.
README.md
Outdated
```sh | ||
pip install -U pydantic-zarr | ||
# or, with zarr i/o support | ||
pip install -U pydantic-zarr[zarr] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find having [zarr]
here a bit odd because in my head a package called pydantic-zarr
would naturally already have support for zarr, but then I guess that's what the Python package is called and [zarr-python]
(my only other idea) is worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) yeah ... I dunno. naming is tough, and this PR indeed somewhat changes the scope of this package (and that of ome-zarr-models-py) to lean more towards the "model"/pydantic side, and less towards the "zarr"/IO side. I think that actually fits nicely with the "zarr as specification, not implementation" concept... but indeed, it could be confusing to the many folks who probably associate zarr-python
with "how you use zarr in python".
your call! just let me know if you want me to change it
Co-authored-by: David Stansby <[email protected]>
Co-authored-by: David Stansby <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 - I'd like @d-v-b to review this before merging too, since it's a pretty major change.
Co-authored-by: David Stansby <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great, thanks @tlambert03 and @dstansby for moving this forward
proof of principle to make the zarr dependency optional. (plenty of space to discuss the exact code patterns, and how the hatch matrix was done)